Codecov Report

Merging #1326 into master will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1326      +/-   ##
==========================================
+ Coverage   93.04%   93.18%   +0.14%     
==========================================
  Files         137      139       +2     
  Lines        3794     3932     +138     
  Branches      784      805      +21     
==========================================
+ Hits         3530     3664     +134     
- Misses        264      268       +4     

I'm frustrated that we didn't come to a consensus at #1325 and there is going to be another place to discuss about the topic.

In this PR, Point was generic parameterized

// Previously
export interface Point {
  value: Sum | LastValue | Distribution | Histogram;
  timestamp: HrTime;
}

// In this PR
export interface Point<T> {
  value: T;
  timestamp: HrTime;
}

// usage
toPoint(): Point<AggregatorTypes>;

Actually, this is no significant difference to exporters: they are still seeing an aggregator's toPoint returning a non-concrete point value types, in the PR, it's type AggregatorTypes = Sum | LastValue | Distribution | Histogram;. So exporters still have to cast the types to access their fields like count.

Notably, in this PR, the as type casting like

assert.deepStrictEqual(
  record1.aggregator.toPoint().value as Distribution,
    {
    count: 0,
    last: 0,
    max: -Infinity,
    min: Infinity,
    sum: 0,
  }
);

was removed to

assert.deepStrictEqual(
  record1.aggregator.toPoint().value,
    {
    count: 0,
    last: 0,
    max: -Infinity,
    min: Infinity,
    sum: 0,
  }
);

This is valid without the changes since it doesn't access the point value fields like

record1.aggregator.toPoint().value.count // this is the problem that going to fail the type check.

This is going nowhere, not sure why but you are not listening

1. In your both PRs you are telling this cannot be done, @vmarchaud posted playground thinking the same at first. I have posted code examples with many explanations that it is easy and can be done, but yet it is not enough. So I created a whole PR with metrics refactoring to prove all I said is true and to show it clearly what I meant, still bad :)

record1.aggregator.toPoint().value.count

2. This example has nothing to with the metrics refactoring neither mine or yours.
   record1 is type of MetricRecord which has aggregator field type of Aggregator
   nor here nor in your 2 PRs you are able to do the same because MetricRecord is a mix of all metrics with different aggregators. No matter what you do you will have to check what kind of metric it is and cast it in some moment. Doesn't matter how many different aggregators interfaces you create, once you get an array of MetricRecord you have to figure out which metric you are dealing with and what aggregator it has, moreover people can create own batchers with own rules and different aggregators for the metrics. Once you call record1.aggregator.toPoint() you have to know what you are dealing with because this is MetricRecord not Metric itself

It is important to point out that typescript does branch type narrowing.

So with PR #1325, we can do:

const metricRecord: MetricRecord = getRecordSomeWhere()

if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
  const point = metricRecord.aggregator.toPoint()
  // this point can be infered as Point<Histogram> without any manual type casting 
  // since aggregator can be determined as type of SumAggregatorType for the `metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM`
}

But with this PR, we still can't do such nice things:

const metricRecord: MetricRecord = getRecordSomeWhere()

if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
  const point = metricRecord.aggregator.toPoint()
  // this point is infered as Point< AggregatorTypes>, so type casting is still needed.
  // accessing `point.value.count` needs a type casting
}

Yes, we have to determine the aggregator type in exporters. But with #1325, we leverage typescript branch type narrowing so the only thing we need to do is to switching case on the aggregator.kind.

So with PR #1325, we can do:

const metricRecord: MetricRecord = getRecordSomeWhere()
if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {
  const point = metricRecord.aggregator.toPoint()
  // this point can be infered as Point<Histogram> without any manual type casting 
  // since aggregator can be determined as type of SumAggregatorType for the `metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM`
}

And this is finally a solid example what exactly you are trying to achieve. Then you right

closing this with regards to last comment

@legendecas please add this explanation how you want to use metric records into your PR
etc if (metricRecord.aggregator.kind === AggregatorKind.HISTOGRAM) {

@obecny Sorry but this is ridiculous. You were outright disrespectful with both your original comments on #1325 and here, you didn't understood what @legendecas was trying to achieve and were insistent on the fact that he was "wrong" then when you get it, its "finally a solid example". Could you please next time refrain to say this when clearly there was a misunderstanding ?

@obecny Sorry but this is ridiculous. You were outright disrespectful with both your original comments on #1325 and here, you didn't understood what @legendecas was trying to achieve and were insistent on the fact that he was "wrong" then when you get it, its "finally a solid example". Could you please next time refrain to say this when clearly there was a misunderstanding ?

wow strong words, but I never meant to be disrespectful, I tried to show many times what I meant with a good will, looks like I was completely misunderstood. If you or @legendecas felt offended then apology it was never my intention.

I am locking this conversation. Thank you @obecny for your apology, even though it was not your intention to offend. With a team this culturally diverse, there are bound to be misunderstandings and I hope we can use this as a learning opportunity.